Add Openplantbook integration #42359

Olen · 2020-10-25T17:52:22Z

Proposed change

This PR adds access to the OpenPlantbook API, and is mostly a preparation for a later PR to the Plant component.
The Openplantbook component itself does not do much, except providing a way for Home Assistant to communicate with the OpenPlantbook API and online DB to fetch data.
There are two service calls which allows you to search for plant species, and get data for a specific species.
https://open.plantbook.io/

This PR is a replacement for PR#42200 where the git history was problematic after a bad rebase.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

The component requires the user to get a client id and secret to the OpenPlantbook API. It is configured using a config flow.
The Openplantbook API is the exposed to HA as a class where other components (e.g. the Plant component) can fetch data.
A PR for the Plant component where it can take advantage of the OpenPlantbook API will be submitted once this component is accepted.

This PR fixes or closes issue: fixes #
This PR is related to issue: PR#42200
Link to documentation pull request: Add documentation of the Openplantbook integration home-assistant.io#15445

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

Olen · 2020-10-26T11:36:38Z

I still don't understand what I can do to fix the codecov/patch issue.

RobBie1221 · 2020-10-27T18:31:32Z

I still don't understand what I can do to fix the codecov/patch issue.

You can check which lines of codes are not hit by tests:
pytest --cov=homeassistant/components/openplantbook/ --cov-report term-missing -- tests/components/openplantbook/test_*.py

RobBie1221 · 2020-10-27T18:37:40Z

You need 100% coverage for at least config flow.

Olen · 2020-10-27T19:31:14Z

Thanks!

Now I have 100% covreage of config_flow, and all 3 tests succeeds locally.

joncar · 2021-01-05T05:39:49Z

homeassistant/components/openplantbook/__init__.py

+
+from .const import ATTR_ALIAS, ATTR_API, ATTR_HOURS, ATTR_SPECIES, CACHE_TIME, DOMAIN
+
+CONFIG_SCHEMA = vol.Schema({DOMAIN: vol.Schema({})}, extra=vol.ALLOW_EXTRA)


Since this is configured via config_flow, the CONFIG_SCHEMA shouldn't be needed.

joncar · 2021-01-05T05:48:30Z

homeassistant/components/openplantbook/__init__.py

+                    species,
+                )
+                wait = 0
+                while "pid" not in hass.data[DOMAIN][ATTR_SPECIES][species]:


asyncio.Event would be a good replacement for this retry loop.

joncar · 2021-01-05T05:53:54Z

homeassistant/components/openplantbook/config_flow.py

+        _LOGGER.debug("Unable to connect to OpenPlantbook: %s", ex)
+        raise
+
+    return {"title": "Openplantbook API"}


Nitpick: There are three different capitalizations of OpenPlantBook in this file. Should they all be replaced with one, correct branding?

joncar · 2021-01-05T05:55:38Z

homeassistant/components/openplantbook/config_flow.py

+                return self.async_create_entry(title=info["title"], data=user_input)
+            except Exception as ex:
+                _LOGGER.error("Unable to connect to OpenPlantbook: %s", ex)
+                raise


An exception while validating input/trying to connect would typically be given back to the user by showing the form again with the the errors field set. This lets the user fix their input and try again. I'm not sure what the user experience of a thrown exception during config flow would be.

joncar · 2021-01-05T05:57:14Z

homeassistant/components/openplantbook/services.yaml

@@ -0,0 +1,13 @@
+search:


The clear_cache service isn't listed here

joncar · 2021-01-05T05:58:45Z

tests/components/openplantbook/test_config_flow.py

+
+async def test_form(hass):
+    """Test we get the form."""
+    await setup.async_setup_component(hass, "persistent_notification", {})


The component doesn't interact with persistent_notification. Does it need to be initialized in the tests?

joncar · 2021-01-05T06:03:24Z

homeassistant/components/openplantbook/__init__.py

+                entity_id = async_generate_entity_id(
+                    f"{DOMAIN}" + ".{}", plant_data["pid"], current_ids={}
+                )
+                hass.states.async_set(entity_id, plant_data["display_pid"], attrs)


I'm not familiar with anything in the plant area so take this comment with a grain of salt, but it feels unusual that services are creating and destroying entities. In the light/switch/sensor/etc world it'd be more traditional for the config_entry to know about what entities will be created and for them all to be created during setup.

joncar · 2021-01-05T06:04:50Z

homeassistant/components/openplantbook/__init__.py

+        hass.data[DOMAIN] = {}
+    if ATTR_API not in hass.data[DOMAIN]:
+        hass.data[DOMAIN][ATTR_API] = OpenPlantBookApi(
+            entry.data.get("client_id"), entry.data.get("secret")


Throughout the component there are magic strings that are used in multiple places. It is very common in HA for these all to be defined as constants so there is less chance for mismatching typos. There are already a bunch of these in const.py but there are many that are not.

github-actions · 2021-04-05T06:06:36Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

dgomes · 2021-05-08T09:45:25Z

@Olen would you address @joncar comments ? I would like to see this being accepted 😄

frenck

This PR has been running stale since 5 Jan, without a response. I think it is fair to say this is really stale and should be closed.

Looking at the PR: It doesn't provide any entities, yet it directly injects/modifies data in the state engine by injecting "fake" entities and data. This is absolutely a no-go.

balloob · 2021-05-09T01:56:36Z

This contribution doesn't fit the Home Assistant model. We represent states, not encyclopedia data.

dgomes · 2021-05-09T10:16:28Z

I agree that information from openplantbook should not become part of the state of the entities, but there is a lot of value in configuring the plant entity with information from openplantbook.

Currently the plant entity yaml exposes all these parameters retrieved from openplantbook as yaml configuration parameters, doesn't it make sense to fill those in automatically? This PR might not have been the best approach, but doesn't the use case make sense ?

balloob · 2021-05-10T05:05:02Z

A config flow step could fetch this information when configuring a plant.

Olen · 2021-05-10T05:55:43Z

A config flow step could fetch this information when configuring a plant.

We tried that approach first, however we needed to add a few more data points to the plant object, and that was rejected since you do not allow "config" in the attributes of an entity.

probot-home-assistant bot added the Hacktoberfest label Oct 25, 2020

homeassistant added the cla-signed label Oct 25, 2020

probot-home-assistant bot added has-tests integration: openplantbook new-integration labels Oct 25, 2020

project-bot bot added this to Needs review in Dev Oct 25, 2020

This was referenced Oct 25, 2020

Openplantbook documentation home-assistant/home-assistant.io#15406

Closed

New component OpenPlantbook #42200

Closed

Olen changed the title ~~Openplantbook new~~ New integration Openplantbook (2. attempt) Oct 25, 2020

springstan changed the title ~~New integration Openplantbook (2. attempt)~~ Add Openplantbook integration Oct 25, 2020

Olen force-pushed the openplantbook-new branch from 375e896 to 287ae2f Compare October 26, 2020 07:31

Olen added 9 commits October 27, 2020 07:50

New initial commit

8a0405e

Fixing test

d539c1b

Remove some debug. Retrying tests

3014987

Adding __init__.py to test

9bcd12e

Adding init to coveragerc

81aa5c5

Cache fetched plant data

deee0ba

Better handling of mulitple requests to the same species

8a9a3da

Add more tests

389f188

Removing "TODO"

99f8025

Olen force-pushed the openplantbook-new branch from b4933f3 to 99f8025 Compare October 27, 2020 06:51

Olen added 2 commits October 27, 2020 14:09

Implementing cache cleanup

fc9ef9e

Fixing test

f81fcad

Also test if config_flow fails

9b681a5

Add keep time as a parameter to clean_cache

efbf5fd

Olen mentioned this pull request Oct 28, 2020

Add documentation of the Openplantbook integration home-assistant/home-assistant.io#15445

Closed

8 tasks

Change days to hours

5bb5aac

PeteBa mentioned this pull request Oct 28, 2020

Add plant limits attributes #42420

Closed

21 tasks

Olen mentioned this pull request Oct 30, 2020

Adding icons for openplantbook home-assistant/brands#1993

Closed

11 tasks

MartinHjelmare added the hacktoberfest-accepted label Oct 31, 2020

cgarwood added this to Incoming in New Integrations via automation Nov 25, 2020

joncar reviewed Jan 5, 2021

View reviewed changes

github-actions bot added stale and removed stale labels Apr 5, 2021

Dev automation moved this from Needs review to Review in progress May 8, 2021

frenck requested changes May 8, 2021

View reviewed changes

New Integrations automation moved this from Incoming to Awaiting Requested Changes May 8, 2021

balloob closed this May 9, 2021

Dev automation moved this from Review in progress to Cancelled May 9, 2021

New Integrations automation moved this from Awaiting Requested Changes to Cancelled May 9, 2021

probot-home-assistant bot added invalid and removed Hacktoberfest labels May 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Openplantbook integration #42359

Add Openplantbook integration #42359

Olen commented Oct 25, 2020 •

edited

Olen commented Oct 26, 2020

RobBie1221 commented Oct 27, 2020

RobBie1221 commented Oct 27, 2020

Olen commented Oct 27, 2020

joncar Jan 5, 2021

joncar Jan 5, 2021

joncar Jan 5, 2021

joncar Jan 5, 2021

joncar Jan 5, 2021

joncar Jan 5, 2021

joncar Jan 5, 2021

joncar Jan 5, 2021

github-actions bot commented Apr 5, 2021

dgomes commented May 8, 2021

frenck left a comment

balloob commented May 9, 2021

dgomes commented May 9, 2021

balloob commented May 10, 2021

Olen commented May 10, 2021


		from .const import ATTR_ALIAS, ATTR_API, ATTR_HOURS, ATTR_SPECIES, CACHE_TIME, DOMAIN

		CONFIG_SCHEMA = vol.Schema({DOMAIN: vol.Schema({})}, extra=vol.ALLOW_EXTRA)

Add Openplantbook integration #42359

Add Openplantbook integration #42359

Conversation

Olen commented Oct 25, 2020 • edited

Proposed change

Type of change

Additional information

Checklist

Olen commented Oct 26, 2020

RobBie1221 commented Oct 27, 2020

RobBie1221 commented Oct 27, 2020

Olen commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2021

dgomes commented May 8, 2021

frenck left a comment

Choose a reason for hiding this comment

balloob commented May 9, 2021

dgomes commented May 9, 2021

balloob commented May 10, 2021

Olen commented May 10, 2021

Olen commented Oct 25, 2020 •

edited